Skip to content

get_differential_vars: check Diagonal.diag, not the full matrix - #3923

Open
singhharsh1708 wants to merge 1 commit into
SciML:masterfrom
singhharsh1708:fix-gpu-diagonal-massmatrix-scalar-index
Open

get_differential_vars: check Diagonal.diag, not the full matrix#3923
singhharsh1708 wants to merge 1 commit into
SciML:masterfrom
singhharsh1708:fix-gpu-diagonal-massmatrix-scalar-index

Conversation

@singhharsh1708

@singhharsh1708 singhharsh1708 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Rework of the original patch, rebased onto current master (0be95df).

The GPU DAE lane has recovered on master without this change. After the CUDA environment rework on the self-hosted runner, GPU: dae_tests.jl stands at 136 passed / 1 failed / 7 broken as of master run 30340807357 (Jul 28); the one remaining failure is Hairer4 [jac=CSC, mass=diag_cu], a different problem. So the earlier "fixes 100% GPU DAE failure" framing no longer applies, and #3922 should be closed or retitled to track the remaining Hairer4 case. What this PR still fixes is the access pattern.

get_differential_vars (lib/OrdinaryDiffEqCore/src/misc_utils.jl) checks the mass matrix with all(!iszero, mm) before its _isdiag branch. For a Diagonal that walks all n^2 positions, and every diagonal hit is a scalar getindex on the underlying vector, which is disallowed by default when that vector is a CuVector, and O(n^2) for a question the type answers in O(n). The fix adds a Diagonal dispatch that broadcasts over mm.diag, so the reduction is O(n) and stays on device. This is the same diagonal-only reduction find_algebraic_vars_eqs(::Diagonal) in the same file performs, though that one goes through diag(M) rather than the field.

CPU results are unchanged. Mixed, all-nonzero and all-zero diagonals return exactly what the old _isdiag path returned, including the reshape to size(u) for matrix-valued u, and all three are covered by new tests.

The access pattern itself is now tested on CPU: a Diagonal wrapping a vector type that errors on scalar getindex but supports whole-array broadcast, i.e. device-array semantics without a GPU. On master's code path that construction throws inside all(!iszero, mm); with this patch it returns the correct differential-vars mask.

One test adjustment was required. The Diagonal branch is statically resolvable, which fixes inference of solve for the two Rodas4 + ShampineCollocationInit cases at lib/OrdinaryDiffEqNonlinearSolve/test/mass_matrix_tests.jl:385/387. Those lines were @test_broken sol = @inferred solve(...), a form that errors with "Expression evaluated to non-Boolean" once @inferred succeeds, because the successful @inferred returns the solution object. Both lines now read @test (@inferred solve(...); true): green while inference holds, an error if it regresses. I checked the replacement form against master's code and it does error there, so it is not a rubber stamp. The Rosenbrock23 and Rodas5 lines are untouched and stay @test_broken; those two fail because solve itself throws CheckInitFailureError, which this change does not affect.

Also wires up show_errors(results) in the GPU DAE test entry point; it was defined but never called, so a failing case only ever logged a status glyph with no exception text. That file runs only on the self-hosted GPU lane.

Local verification on Julia 1.12.6, rebased on master:

  • lib/OrdinaryDiffEqCore/test/algebraic_vars_detection_tests.jl: 22 passed / 0 failed (18 on master, so 4 new assertions).
  • lib/OrdinaryDiffEqNonlinearSolve/test/mass_matrix_tests.jl: 227 passed / 0 failed / 0 errored / 40 broken. Master baseline on the same file is 225 passed / 42 broken; the two that flip are the Rodas4 lines above.
  • lib/OrdinaryDiffEqBDF: dae_convergence_tests.jl 50/50, dae_initialization_tests.jl 38 passed / 2 broken (pre-existing).
  • Runic 1.5.1 clean on all four changed files.

No GPU hardware here, so the CUDA path is covered by the CPU device-semantics guard test rather than on-device.

The red checks on this PR are inherited from master. format-check fails on lib/OrdinaryDiffEqCore/src/disco.jl, lib/OrdinaryDiffEqCore/src/integrators/controllers.jl and test/Integrators_I/disco_tests.jl, none of which this PR touches and all of which are dirty in a clean master checkout. The lib/OrdinaryDiffEqNonlinearSolve Core lane dies in linear_solver_tests.jl with UndefVarError: MatrixOperator before it ever reaches mass_matrix_tests.jl; the same failure is on master run 30610465511. Both need fixing separately.

AI Disclosure

Claude assisted with this work.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

The GPU job on this PR gets past the get_differential_vars path, but it exposes a second independent blocker in the same Simple DAE GPU test: 87623782250 fails at simple_dae.jl:54 with DimensionMismatch: array could not be broadcast to match destination. The first package frame is the three-argument ldiv!(x::CuMatrix, qr::QR, b::CuMatrix) in cuSOLVER linalg.jl:301; x and b are both matrix right-hand sides. That exact upstream defect and regression are already addressed by CUDA.jl #3199.

This appears to confirm that this PR cleared the earlier scalar-indexing barrier, after which CI reached the unrelated QR barrier. I would keep this PR focused; its GPU job cannot become green until the CUDA fix is available in the resolved environment.

@singhharsh1708
singhharsh1708 force-pushed the fix-gpu-diagonal-massmatrix-scalar-index branch from 8281897 to ff724e9 Compare July 18, 2026 07:32
@singhharsh1708
singhharsh1708 force-pushed the fix-gpu-diagonal-massmatrix-scalar-index branch from ff724e9 to 59b1424 Compare July 21, 2026 14:31
@singhharsh1708
singhharsh1708 force-pushed the fix-gpu-diagonal-massmatrix-scalar-index branch from 59b1424 to aaef682 Compare July 30, 2026 21:25
@singhharsh1708 singhharsh1708 changed the title get_differential_vars: check Diagonal.diag, not the full matrix (fixes GPU DAE 100% failure) get_differential_vars: check Diagonal.diag, not the full matrix Jul 30, 2026
@singhharsh1708
singhharsh1708 force-pushed the fix-gpu-diagonal-massmatrix-scalar-index branch from aaef682 to 3410aad Compare July 31, 2026 09:19
all(!iszero, mm) visits every (i, j) pair of the mass matrix via
scalar getindex. For a GPU-backed diagonal mass matrix
(Diagonal{Float64, <:CuVector}) that indexing is disallowed by
default, so the check throws whenever it runs on a device array,
and it is O(n^2) for a structurally O(n) question. Diagonal
guarantees off-diagonal zeros, so check mm.diag directly; the
broadcast over the diagonal vector dispatches to a device kernel,
matching what find_algebraic_vars_eqs(::Diagonal) already does.

Adds a guard test: a Diagonal wrapping a vector that errors on
scalar getindex but supports whole-array broadcast, i.e. device
array semantics on the CPU, so a regression back to the
whole-matrix walk is caught without GPU hardware in CI.

The statically resolvable Diagonal branch also fixes inference of
solve for the two Rodas4 + ShampineCollocationInit cases in the
NonlinearSolve mass matrix tests. Their form
@test_broken sol = @inferred solve(...) errors with "expression
evaluated to non-Boolean" once @inferred succeeds, so those two
lines become @test (@inferred ...; true), still erroring if
inference regresses.

Also wires up show_errors(results) in the GPU DAE test entry
point, which was defined but never called, so a real failure
prints its actual exception instead of just a status glyph.
@singhharsh1708
singhharsh1708 force-pushed the fix-gpu-diagonal-massmatrix-scalar-index branch from 3410aad to 806ae08 Compare July 31, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants